Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add env variables to change UID/GID of www-data #1278

Closed
wants to merge 1 commit into from
Closed

Add env variables to change UID/GID of www-data #1278

wants to merge 1 commit into from

Conversation

jan-di
Copy link

@jan-di jan-di commented Oct 17, 2020

This PR adds the support for two environment variables that can change the UID/PID of the www-data user dynamically at the entrypoint script. This allows to use a specific uid/gid that is also used on the host to prevent permission issues on bind mounted volumes.

To stay backwards-compatible, the environment variables have the current UID/GID as default value, so they are 3 possible use cases:

  • No UID/GID defined, no --user parameter given: Image starts as root, apache/php uses default UID/GID (as before)
  • No UID/GID defined, --user parameter given: Image starts as $user, apache/php uses default UID/GID (as before)
  • UID/GID defined, no --user parameter given: Image starts as root, apache/php uses defined UID/GID (new)

An additional advantage of changing the UID/GID of www-data instead of creating a new user is, that no config files have to be changed for this - every service can use www-data as before.

This should fix #359, #1081, #1087. See also #772 (comment) for more info.

@PrivatePuffin
Copy link

I think this is a rather clean solution to the issues at hand, nice work! :)

@jan-di
Copy link
Author

jan-di commented Nov 1, 2020

Hello @tilosp, i have rebased this PR to the latest changes. Some tests are failing due to database tests, but it seems rather random to me. Could I request a review or is there something other I should do before?

@uchagani
Copy link

@tilosp can we get this merged please?

Signed-off-by: Jan Dittrich <mail@jand.one>
@J0WI
Copy link
Contributor

J0WI commented Dec 24, 2020

I'd prefer to use the same approach as in docker-library/wordpress#249 (see also docker-library/wordpress#351 and docker-library/wordpress#371)

@PrivatePuffin
Copy link

@J0WI The problem with the Wordpress (and LinuxServer) design, is the fact is isn't compatible with the docker "user" setting. For those two it's acceptable, because the settings is relatively new but for new implementations, like this one, not being able to use docker best-practices is a no-go.

@J0WI
Copy link
Contributor

J0WI commented Dec 28, 2020

@Ornias1993 do you refer to something else than docker run --user?

@olivergg
Copy link

An alternative solution (or dirty hack ? 😅 ) would be to use bindfs (on Linux) to mirror the directory on your host and set the permission accordingly, then just use this directory instead of the original one in your docker volume

For example, if you have a MyFiles directory with the UID/GID 1000:1000 then you can create a mirror with the UID/GID changed to 33:33 (www-data by default).

# bindfs -u 33 -g 33 MyFiles MyFilesMirrored

See http://manpages.org/bindfs

@eathtespagheti
Copy link

Hoping to see this implemented ASAP

@Fel-o
Copy link

Fel-o commented Aug 17, 2021

any updates?

@djak250
Copy link

djak250 commented Feb 4, 2022

Is this still on people's radar?

@tushev
Copy link
Contributor

tushev commented Feb 5, 2022

I ended up with building my own, custom Docker image, with

RUN addgroup --gid 1000 --system nextcloud
RUN adduser --uid 1000 --system --disabled-login --disabled-password --gid 1000 nextcloud

And my docker-compose.yml contains

  apache:
    build:
      context: ./images
      dockerfile: Dockerfile-nextcloud-apache
    user: '1000:1000'

@bllngr
Copy link

bllngr commented Feb 11, 2022

@Kami @tilosp @J0WI, it seems like this is a clean solution to a frequently requested feature and base for other functionality. It also seems to align with Docker best-practices. But there hasn't been any feedback for one year. Is there anything that speaks against rebasing and merging it?

wasbeer referenced this pull request in wasbeer/nextcloud-docker Feb 20, 2022
wasbeer added a commit to wasbeer/nextcloud-docker that referenced this pull request Feb 20, 2022
@vincentDcmps
Copy link

I'd prefer to use the same approach as in docker-library/wordpress#249 (see also docker-library/wordpress#351 and docker-library/wordpress#371)

in case of you use ldap for connection wordpress approach doesn't work because tou UID is not in /etc/passwd

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! There is something that bother me in this pr, it's the chown and chgrp.
This could lead to multiple issues and I don't think that is the job of a docker image.
The sysadmin should know and manually do those changes.

As a side note:

  • Please fix tests
  • Please only edit the root templates docker-entrypoint.sh. The changes are done automatically
  • Could you explain the requirements for the shadow dependency on alpine ? @jan-di

@skjnldsv skjnldsv requested a review from J0WI June 20, 2022 07:24
@PrivatePuffin
Copy link

PrivatePuffin commented Jul 9, 2022

Hey! There is something that bother me in this pr, it's the chown and chgrp. This could lead to multiple issues and I don't think that is the job of a docker image. The sysadmin should know and manually do those changes.

As a side note:

  • Please fix tests
  • Please only edit the root templates docker-entrypoint.sh. The changes are done automatically
  • Could you explain the requirements for the shadow dependency on alpine ? @jan-di

PUID/UID GID/PGID is quite globally used (and not a very good idea considering systems like kubernetes and docker already implement more secure user-setting, for example the docker --user flag or kubernetes runAsUser.). It's goal is 50% setting the user/group and 50% setting the file permisisons.

However, the design here is actually troublesome:
No UID/GID defined, --user parameter given: Image starts as $user, apache/php uses default UID/GID (as before)

When --user is set, everything should run as that user. Not run as an arbitrary other/default UID/GID. Regardless of what the UID/GID settings are.

Remember UID/GID env-vars are not a standard! Setting the --user or runAsUser is. So the image should focuss on those users, rather than users using UID/GID.

So in the following case:
No UID/GID defined, --user parameter given: Image starts as $user, apache/php uses default UID/GID (as before)

Applications should be running with $user and group.

J0WI added a commit to J0WI/docker-nextcloud that referenced this pull request Sep 1, 2022
fix: nextcloud#359, nextcloud#772, nextcloud#1081, nextcloud#1087, nextcloud#1278

Signed-off-by: J0WI <J0WI@users.noreply.github.com>
J0WI added a commit to J0WI/docker-nextcloud that referenced this pull request Sep 1, 2022
fix: nextcloud#359, nextcloud#772, nextcloud#1081, nextcloud#1087, nextcloud#1278

Signed-off-by: J0WI <J0WI@users.noreply.github.com>
J0WI added a commit that referenced this pull request Sep 6, 2022
fix: #359, #772, #1081, #1087, #1278

Signed-off-by: J0WI <J0WI@users.noreply.github.com>

Signed-off-by: J0WI <J0WI@users.noreply.github.com>
@J0WI
Copy link
Contributor

J0WI commented Sep 6, 2022

closing due #1812

@J0WI J0WI closed this Sep 6, 2022
ananace pushed a commit to ananace/docker-nextcloud that referenced this pull request May 10, 2024
fix: nextcloud#359, nextcloud#772, nextcloud#1081, nextcloud#1087, nextcloud#1278

Signed-off-by: J0WI <J0WI@users.noreply.github.com>

Signed-off-by: J0WI <J0WI@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow the UID/GID to be changed